Skip to content

Have Duration return same units as Postgrex.Interval #728

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 1, 2025

Conversation

greg-rychlewski
Copy link
Member

Based on our discussion

@endoooo
Copy link

endoooo commented Feb 1, 2025

should it be reflected in README? something like:

PostgreSQL Elixir
interval %Duration{month: 2, day: 5, second: 0, microsecond: {315, 6}}

@greg-rychlewski
Copy link
Member Author

good call =)

@greg-rychlewski greg-rychlewski merged commit 928e43a into elixir-ecto:master Feb 1, 2025
9 checks passed
@Wigny
Copy link

Wigny commented Jun 3, 2025

@greg-rychlewski, do we have somewhere an explanation on why this change was made?

I'm curious, as I arrived here by trying to understand why an inserted duration is returned in a different shape than what is given:

iex> Repo.insert!(%Activity{duration: Duration.new!(minute: 25)}, returning: true)
%Activity{duration: %Duration{second: 1500, microsecond: {0, 6}}}

@greg-rychlewski
Copy link
Member Author

greg-rychlewski commented Jun 3, 2025

I will try to find the discussion when I get home. I think it was in another PR. But the gist is that Postgres wire protocol cannot support the same units as elixir duration. And we have to pick a strategy. One is do the same as the old interval struct (which we chose). Another is to fill up the bigger units first and then fill with 0. We started with the latter and changed it to the former based on a discussion with a user

@greg-rychlewski
Copy link
Member Author

We got lucky the discussion was easy to find #688 (comment)

@Wigny
Copy link

Wigny commented Jun 4, 2025

Got it, thanks!

While I don't see an issue with the approach of returning the same units as Postgres (as PT25M and PT1500S should be the same anyway), I wonder if should we have a more friendly way of ensuring these two values are indeed the same (think of when testing insertions, for example)... Maybe having a Duration.compare/2 would solve it, but also rescaling the units would suppress this needing (?).

Suggestion on how to tackle it? Is creating a custom type the way to go or do you think worth suggesting the addition of Duration.compare/2 in the standard library?

@greg-rychlewski
Copy link
Member Author

greg-rychlewski commented Jun 4, 2025

It looks like the docs for duration have some suggestions for comparing so I’m not sure such a function would be accepted into the standard lib. @josevalim is in a better position to speak about that though.

In terms of a custom ecto type, I probably wouldn’t do it just for comparing equality. That could probably be handled with a helper function. At least I cannot think of any reason right now to do that.

@Wigny
Copy link

Wigny commented Jun 4, 2025

It looks like the docs for duration have some suggestions for comparing

Oh, nice.

I probably wouldn’t do it just for comparing equality.

Yeah, I figured also that having a helper would be simpler.

Many thanks.

@greg-rychlewski
Copy link
Member Author

No worries, thank you for raising this. In case you run into it later, the same kind of thing happens with ranges. Postgres will normalize the values you give and then you lose the original ones

@josevalim
Copy link
Member

The thing about comparing durations is that you can only effectively compare them if you have a specific point in time. 30D and 1MO are only equal if the current month has 30 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants